Repository navigation
Conversation
Remove unused imports, sort imports, replace constant setattr with attribute assignment, convert to f-strings, and other safe fixes.
Drop dead assignments (dd_, rst_time, time, ymdh, etc.), discard unused communicate() stderr results, and keep p.wait() calls without binding their return codes.
exit() is only guaranteed by the site module. Behavior is unchanged: exit(msg) and sys.exit(msg) both print msg to stderr and exit with 1.
Replace manual open()/close() with with-blocks. In remap_analysis, read the source file directly instead of via the global fileinput singleton, which was never closed. remap_bin2nc previously never closed its yaml handle.
Convert percent formatting to f-strings, merge nested ifs, return conditions directly, drop redundant .keys() and dict(), and merge isinstance calls. No behavior change.
mathomp4
added
the
0 diff
|
@mathomp4 : I added info about v14 into "message_bc_other". This is just a message that's printed, although it may be confusing that it includes "v14" documentation when "v14" is in the "ops" menu and not in the "other" menu. Perhaps we should also add similar info about the old "ops" bcs ("NL3" etc) in "message_bc_other"? That'd require some thought because a shorthand description for the old "ops" bcs does not yet exist. Let me know if you I should give this a try. FYI, the descriptions are from make_bcs, which is in a different repo |
|
PS: To answer some of your questions: The v12 description is correct. Like I said, the original intent was to keep stuff like v14 in "other". I wouldn't be opposed to moving it there, but I don't insist. I'm not sure if the "testing only" entries are 100% accurate. I don't think these bcs were ever made the default, so probably yes. But I didn't verify. Having said that, I don't think this matters much. In last commit, I reduced the version info to major/minor and dropped the bug fix (third) digit for simplicity. Finally, I don't think we want to provide technical/science descriptions with the "ops" bcs choices. If we include some detail, I would include it in "message_bc_other" for reference. I hope I'm not forgetting anything |
|
@mathomp4 : Does your "thumbs up" emoji in response to my previous comment mean that I should take a crack at adding brief descriptions of old "ops" bcs into "message_bc_other"? Please let me know, thanks |
Oh. I thought you did that in 92db16b Did you mean you might have more you'd like to add? If so, please do! |
|
@mathomp4 , @biljanaorescanin :
|
|
Re. my previous commit: We could conceivably repeat the "ops" bc choices within the "other" bc choices list. After my commit, users only get to select the "ops" choices in the "ops" list, but they see descriptions of the "ops" bcs in the Table shown with the "other" choices. Thoughts? |
@gmao-rreichle as it is currently when you run interactively you'll first see: if you choose other then: It's definitely completek though I guess a bit odd that you can't select some in the second table. @biljanaorescanin what are your thoughts? |
My understanding is that GM4 (Ganymed-4_0) is retained mainly for remapping archived MERRA-2 restarts, since MERRA-2 uses GM4 BCs. This is handled explicitly in init_merra2(), where the input BC version is set to GM4. GM4 predates ICA, and they are different BC generations (mostly a lot of bug fixes, mask changes and formating changes I have listed in notes). I don't think we need GM4 in the interactive dropdown for output BCs, since we generally remap from GM4 to newer versions rather than to GM4. I would keep GM4 supported for command-line remapping and the MERRA-2 input workflow, but wouldn't add it as an output choice unless there is a specific use case for generating GM4 compatible restarts. |
…l"; removed separate explicit list of BCs for command line (remap_utils.py, remap_questions.py, remap_command_line.py, remap_catchANDcn.py)
|
@mathomp4 , @biljanaorescanin : My last two commits are an attempt to standardize and clean up. Specifically:
Alas, my commit fails some "ruff" checks. The first failure is because a list of variables is not sorted alphabetically. The list is ordered meaningfully, and it should not be resorted alphabetically. Not sure how to get ruff to relent. Please take a look and let me know if you agree with the changes. If so, Biljana can restart the testing. |
@gmao-rreichle What exactly do you mean? Python has no notion of imports having an order. Imports are just imports and you can technically specify them in any order. That isn't a "real" Python tuple or list, just syntactic sugar to allow us to not do a But if you want, I can tell ruff to not care about order in that import list. |
|
@mathomp4 : Here's are the linter errors to which I was referring: https://github.com/GEOS-ESM/GEOS_Util/actions/runs/38940178729/job/117105533406?pr=280 I didn't look too closely, but it sounded like "ruff" was complaining about an "un-sorted" import block. Perhaps it's on us to just ignore the errors and merge anyway. I just noticed that there's a failed check in the list of CI tests: 
|
Yes, I know. We are "violating" the style that |
|
One more question for @biljanaorescanin : What is the status of "v15_BETA"? I don't see it in the bcs_shared dir (although I admittedly didn't look very hard). Also, since v14 still uses the old (v1) atmos topography, does this mean that the v12 GCM (and thus MERRA-3) won't be using the new topography????? Do we need to remove "v15_BETA" from the list if it's not in the bcs_shared dir? |
Status is we need to have stable GCMv12 version so I can work with Nathan on tuning. Just 2 weeks ago we moved to v14 BCs... before this we were on v12... so we have : |
So you're saying that v15_BETA does not even exist at this time, right? In that case, we need to remove it from the drawdown list! Please confirm. Looking ahead, this also means that (i) we may not really want to have "v14" in the "ops" list of bcs, if it's going to be superseded in short order, and (ii) I might be able to shore up my retirement funds by putting some money on the v12 GCM not being ready by the end of the year. The problem with the latter, of course, is that I don't see who would want to be betting against me ;) |
gmao-rreichle
left a comment
gmao-rreichle
left a comment
There was a problem hiding this comment.
@biljanaorescanin , see comments below
| "message": message_bc_all_new, | ||
| "choices": choices_bc_all, | ||
| "when": lambda x: x["output:shared:bc_version"] == 'Other' and x["input:shared:bc_version"] not in ['v11', 'v12', 'v13', 'v14', 'v15_BETA'], | ||
| "when": lambda x: x["output:shared:bc_version"] == 'Other' and x["input:shared:bc_version"] not in ['v11', 'v12', 'v13', 'v14'], |
There was a problem hiding this comment.
@biljanaorescanin : I'm trying to understand what exactly the "when:" field does here. What's confusing me is that the list of "not in" bcs doesn't line up with what in the "ops" vs. "all" lists. For starters, we probably need to replace 'Other' with 'All', which I probably forgot to do when I made the corresponding naming change last week. But do we also have to remove "v14" from the list here because it's in the "ops" bcs list? And, in any case, can we avoid the hard-coded list of bcs here?
There was a problem hiding this comment.
Yes, the Otherto All rename needs to be carried through to the when conditions. I found three remaining checks in remap_questions.py and one in remap_catchANDcn.py, so we should update the input and land-only paths as well.
The when field determines whether Questionary shows or skips a question based on the answers collected so far. Here, the two expanded output questions offer the same BC choices, the difference is whether they display the full description table or just the short prompt. The hard coded input-version lists appear intended to avoid repeating that table, rather than restrict which BCs can be selected.
Since the short and complete lists now overlap, the input BC version alone no longer tells us whether the user has already seen the complete table. For example, v14 can be selected directly from the short menu or through All.
Rather than adjust the conditions by removing v14, I suggest replacing those two expanded output questions with a single question using message_bc_all_new and choices_bc_all, shown whenever the output selection is All. That would keep the short menu unchanged and remove the hard coded BC-version lists from those conditions.
The tradeoff is that users selecting All for both input and output would see the detailed table twice.
Would that approach work for you?
|
The tests are not zero-diff because they have not been rebased after the ESMF update.
Both
If we use the old build with the old ESMF, all tests are zero-diff. => We need to rebase. So I think we can proceed with rebasing and merging this PR. After that, I will work on the testing-package issue Matt created: That issue will probably require some additional discussion about what we want from the testing package. For example, we may also want to revisit which BCs are used in testing and whether additional tests are needed. However, I think that work should be handled independently of this PR. |
Summary
Cleanup of the Python code in
pre/remap_restartdriven byruffandty, plus a few bug fixes found along the way, plus making v14 the default BCs version in the interactive questions.Testing: zero-diff. Remaps were run on Discover from this branch and the results were identical to
main.Code cleanup (ruff)
Ruff went from 116 findings to 0.
setattr-> attribute assignment.exit()->sys.exit().remap_bin2ncyaml handle was never closed).from module import *everywhere with explicit imports, so linters and type checkers can now see undefined names..keys()/dict()removal).pre/remap_restart/ruff.toml.__pycache__/,*.py[cod],.ruff_cache/,.pytest_cache/,.venv/) moved to the root.gitignore.Behavior changes
subprocess.call->subprocess.run(..., check=True)for the work steps (dyn2dyn.x,cp,chmod,qsub/sbatch).os.system(script > log 2>&1)is replaced bysubprocess.runwith the log redirected the same way. A failed step now stops the run with aCalledProcessErrorinstead of continuing silently.rm -rfcleanups and thediffcheck inremap_base.pyare unchanged.InData/,OutData/,upper_data/,ana_data/. On failure they are left for debugging.remap_params.yamlis written without line wrapping, which fixes a trailing space afterbc_base:(ruamel.yaml wraps at 80 columns).Bugs fixed
shutil.removedoes not exist; nowos.remove(3 places inremap_other_restarts.py). It would raiseAttributeErroron a rerun into an existing directory.'Ganymed' in bc_versionraisedTypeErrorifbc_versionwas missing; the lookup now defaults to''.get_resolutionscrashed onNoneifagridorogridwas missing; it now raises a clearValueError. All callers seen pass both.tyflagged these;ruffcould not, since it doesn't check module attributes.New default BCs version: v14
In the interactive questions, v14 is now the default and is listed in the main menu. v12 stays in the main menu. v12 and v14 are removed from the "Other" submenu so they aren't listed twice. The command-line
-bcvin/-bcvoutchoices are unchanged.Main menu table, before:
Choices:
v12(default),NL3,ICA,Other. "Other" offeredv11,v12,v13,v14,v15_BETA.Main menu table, after:
Choices:
v14(default),v12,NL3,ICA,Other. "Other" now offersv11,v13,v15_BETA.Notes for reviewers: v12 became the default in GEOSgcm v11.10.0 and v14 in v11.11.0, hence the new GCM ranges. The ADAS column for v14 and v12 is still "future". The v12 end tag (
11.10.x) is a placeholder, so please check it.CI
New workflow
.github/workflows/python-lint.ymlruns on PRs touchingpre/remap_restart:ruff(blocking)ty(blocking). It is clean after theget_resolutionsfix below.Not changed
ruff format/ indentation normalisation (the code mixes 2/3/5-space indents) is left for a separate PR.